-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make UI test annotations mandatory #11421
Make UI test annotations mandatory #11421
Conversation
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am opposed to this merging with each test having the full text of the error in it. This leads to a lot more test churn. I think this work needs to be done carefully, looking at each test and seeing what stuff should be in the test file and what stuff should just go in the ui output file.
cc @rust-lang/clippy we should probably discuss this as a team
Posted more context on zulip)
☔ The latest upstream changes (presumably #11418) made this pull request unmergeable. Please resolve the merge conflicts. |
So from the discussion on zulip, it was decided that only errors and the clippy lint name should be generated. But errors should still be present in UI test files. |
I don't know what you mean by this, can you link to the actual conclusion? |
Sorry, forgot to add it. It's here. |
Ah. Right, I remember that. The proposal is to autogenerate annotations that have the lint name, only for errors, and then mandate them. Sure. |
Exactly. I'll need to update |
There's oli-obk/ui_test#165 for matching lint names |
Ah nice. But that's not the only thing needed. I'm working on something else needed too which I described in my previous comment. |
My impression from the meeting was that we don't plan to have |
Yes but we need to take into account that some existing UI tests have help/note annotations. |
So this PR is now waiting for oli-obk/ui_test#165 and for oli-obk/ui_test#182 to be merged. EDIT: oli-obk/ui_test#182 allows to not change the minimum annotation level (in our case, we don't want to make annotations below "error" level mandatory) and oli-obk/ui_test#165 allows to set the lint name as annotation message instead of the actual message. |
I would imagine that we will mass replace the existing patterns with oli-obk/ui_test#165 style ones before making them mandatory, at which point we can also remove the |
Why removing them? They're already here so better keep them no? |
People look at existing test files for inspiration when writing their own so we'd want to avoid having multiple different styles across the test suite |
Hey @GuillaumeGomez, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation? If you have any questions, you're always welcome to ask them in this PR or on Zulip. @rustbot author |
Yes I am. Did the new version of |
I don't know, can you maybe check once you're back home? If not we can ask in the repo and see what needs to be done to move this forward :D |
I'll check when I'm back then and publish the update here once done. 👍 |
Hey @GuillaumeGomez, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation? If you have any questions, you're always welcome to ask them in this PR or on Zulip. @rustbot author |
Ah right, need to check if the update was done. Thanks for the ping! |
42424fc
to
3917a29
Compare
…we find a solution for `tests/ui-internal/custom_ice_message.rs`
f14a064
to
a50953c
Compare
This review is outdated (~1½ y/o). It is no longer required to add the full your message and the lint name is enough. We discussed this in multiple threads on Zulip and came to the decision, that we want to enforce these annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this merged! Thanks for doing this!
Argh, the 32 bit tests, I also always forget about those too. |
1f97359
to
8ae4e7f
Compare
64a249f
to
7a245c6
Compare
7a245c6
to
847bd67
Compare
Follow-up of #11249.
changelog: make UI tests annotations mandatory